Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bump libunwind more #2164

Merged
merged 14 commits into from
Jun 17, 2021
Merged

bump libunwind more #2164

merged 14 commits into from
Jun 17, 2021

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Jun 15, 2021 •
edited
Loading

Signed-off-by: Schrodinger ZHU Yifan [email protected]

What problem does this PR solve?

Problem Summary:

#2156 does not fix the issue for x86_64 platform. As it is mentioned in libunwind/libunwind#166, _U_dyn_info_list is defined multiple times due to gcc-10's changes in the weak symbol behavior. In this PR, we nuke the self-vendor-ed libunwind and follow upstream changes by using git submodules. This will fix the issue. Meanwhile, I also baked the support for aarch64.

What is changed and how it works?

Proposal: xxx

What's Changed:

  • libunwind removed
  • submodule libunwind registered (follow master directly, v1.5 is another choice but seems that master has better support with aarch64)

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch:

Check List

Tests

  • Unit test
  • Integration test

Release note

No release note

@SchrodingerZhu
bump libunwind more
25afc97 
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@SchrodingerZhu
follow upstream
3678be7 
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
fix ci
1b4ef80 
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
fix ci
fbfd018 
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
fix ci
5f0b008 
Signed-off-by: Schrodinger ZHU Yifan <[email protected]>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@SchrodingerZhu
enable unwind on default (aarch64)
d518f16 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
fix build for other arch
4c97974 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
remove extra space
d901302 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
suppress warning
1b6f401 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
fix aarch64 context
3f40276 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
fix aarch64 context (more)
474409e 
Signed-off-by: SchrodingerZhu <[email protected]>
@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

@SchrodingerZhu
Copy link
Contributor Author

PTAL @fzhedu @flowbehappy . this PR also enables aarch64's unwinding.

@SchrodingerZhu SchrodingerZhu mentioned this pull request Jun 16, 2021
Open
flowbehappy
flowbehappy previously approved these changes Jun 16, 2021
View reviewed changes
@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 16, 2021
@flowbehappy flowbehappy self-requested a review June 16, 2021 08:02
@flowbehappy flowbehappy dismissed their stale review June 16, 2021 08:03

Compilation on mac failed

@flowbehappy
Copy link
Contributor

Compilation failed on Mac.

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[ 14%] Building CXX object contrib/poco/Foundation/CMakeFiles/Foundation.dir/src/DateTimeParser.cpp.o
Scanning dependencies of target unwind
[ 14%] Building C object contrib/libunwind-cmake/CMakeFiles/unwind.dir/__/libunwind/src/dwarf/Lexpr.c.o
In file included from /Users/flow/workspace/github/theflash/storage/ch/contrib/libunwind/src/dwarf/Lexpr.c:4:
In file included from /Users/flow/workspace/github/theflash/storage/ch/contrib/libunwind/src/dwarf/Gexpr.c:26:
In file included from /Users/flow/workspace/github/theflash/storage/ch/contrib/libunwind/include/dwarf_i.h:9:
/Users/flow/workspace/github/theflash/storage/ch/contrib/libunwind/include/dwarf.h:43:14: fatal error: 'link.h' file not found
    #include <link.h>
             ^~~~~~~~
1 warning generated.
1 error generated.
make[3]: *** [contrib/libunwind-cmake/CMakeFiles/unwind.dir/__/libunwind/src/dwarf/Lexpr.c.o] Error 1
make[2]: *** [contrib/libunwind-cmake/CMakeFiles/unwind.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....

@leiysky
Copy link
Contributor

leiysky commented Jun 16, 2021

Verified can work on my Ubuntu.

LGTM except the macOS issue.

@SchrodingerZhu
Copy link
Contributor Author

/run-all-tests

flowbehappy
flowbehappy approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@flowbehappy flowbehappy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SchrodingerZhu
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 17, 2021
@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot ti-srebot merged commit 47bbbb4 into pingcap:master Jun 17, 2021
@JaySon-Huang JaySon-Huang deleted the libunwind-fix branch October 15, 2021 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flowbehappy flowbehappy flowbehappy approved these changes

@JaySon-Huang JaySon-Huang Awaiting requested review from JaySon-Huang

@fuzhe1989 fuzhe1989 Awaiting requested review from fuzhe1989

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SchrodingerZhu @flowbehappy @leiysky @ti-srebot